-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MIWI API endpoints #3608
MIWI API endpoints #3608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly lgtm! Just a few questions / comments. thanks!
pkg/api/admin/platformworkloadidentityroleset_validatestatic.go
Outdated
Show resolved
Hide resolved
out.Properties.PlatformWorkloadIdentityRoles[i].ServiceAccounts = make([]string, 0, len(r.ServiceAccounts)) | ||
out.Properties.PlatformWorkloadIdentityRoles[i].ServiceAccounts = append(out.Properties.PlatformWorkloadIdentityRoles[i].ServiceAccounts, r.ServiceAccounts...) | ||
for _, r := range s.Properties.PlatformWorkloadIdentityRoles { | ||
role := PlatformWorkloadIdentityRole{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space between PlatformWorkloadIdentityRole
and {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put a space before the curly brace if for example this were line 23 and I had left out the space after the range
expression, but I think it's normal Go style to avoid leaving blank space in that spot in a struct literal, and that's what's done throughout the codebase.
for i, r := range new.Properties.PlatformWorkloadIdentityRoles { | ||
if r.OperatorName == "" { | ||
errs = append(errs, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].operatorName", i), "Must be provided")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of comparing struct fields individually, consider using this: https://github.com/go-playground/validator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I played around with it, but I don't think it's good for our static validators simply because it doesn't give the user friendly error messages that we want going to those making API requests. For example:
Key: 'PlatformWorkloadIdentityRole.OperatorName' Error:Field validation for 'OperatorName' failed on the 'required' tag
This error message would be perfectly fine for me as a regular developer of the RP, but it doesn't even include the fully qualified path of the API field presenting the issue, which won't be great for external users. (Well, I say "external users", but this is the admin API... 🙂)
I think adding it in just for this PR would create an inconsistency with respect to our error messaging and that with that in mind it would be best considered as a future refactor that we do across all of the static validators. Your thoughts?
I agree that patch seems unnecessary. This PR is already large, if we decide we need patch after a while then let's do it in its own PR. Still reviewing code may have more comments later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
half done review for discussion points.
a6c513a
to
9607267
Compare
pkg/frontend/admin_platformworkloadidentityroleset_list_test.go
Outdated
Show resolved
Hide resolved
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run ci |
I originally wrote the code the way I did so that we could aggregate errors so that we could provide a better UX in cases where there are multiple similar errors in the request content. I found while writing unit tests that aggregating the errors in this way and not wrapping them in a CloudError causes the RP to return an internal server error instead of a 400 bad request. Is there a way we can aggregate the errors and still wrap them in a CloudError? I'm not sure of the formatting requirements for the text of CloudErrors.
…ame time where applicable for better UX
… minor version more readable
bae3131
to
d7e58e2
Compare
/azp run ci, e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left one final comment I honestly don't know the answer to - just want to get a thought out there. Great work!
if err != nil { | ||
t.Fatal(err) | ||
} | ||
oc := tt.preflightRequest() | ||
|
||
go f.Run(ctx, nil, nil) | ||
f.mu.Lock() | ||
f.ocpVersionsMu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct - I'm just trying to wrap my head around concurrent writes to different tables. I understand that from a Cosmos perspective, we should be able to write to both containers at the same time, but my question is, can the RP handle that? Would it be safer code-wise to keep all locks as the same thing, even if it is inefficient, so we don't risk a future change mistakingly using the wrong lock, because the RP doesn't write enough to Cosmos for us to care? ... Maybe I'm over-thinking this. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-6448
What this PR does / why we need it:
This PR introduces the API endpoints for the
PlatformWorkloadIdentityRoleSet
proxy resources as discussed in MIWI design documents. The only deviation from the design docs is that I did not implement an adminPATCH
- it's extra work, and we are hardly if ever going to use it (refer toupdate_ocp_versions.go
to see how we doOpenShiftVersions
today and talk directly to Cosmos DB without even using thePUT
API endpoint). But if the team reviews and thinks aPATCH
is worth adding, we can add it.The three new endpoints are:
GET
(list)GET
(list)PUT
Test plan for issue:
Is there any documentation that needs to be updated for this PR?
No (I think?)
How do you know this will function as expected in production?
Tested the endpoints with a full service RP, reused existing battle-tested frontend code, and was thorough in writing unit test cases.